Skip to content

Update documentation for Arc::from_raw, Arc::increment_strong_count, and Arc::decrement_strong_count to clarify allocator requirement #134496

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jan 16, 2025
Merged

Update documentation for Arc::from_raw, Arc::increment_strong_count, and Arc::decrement_strong_count to clarify allocator requirement #134496

merged 2 commits into from
Jan 16, 2025

Conversation

DiuDiu777
Copy link
Contributor

@DiuDiu777 DiuDiu777 commented Dec 19, 2024 •
edited
Loading

Related Issue:

This update addresses parts of the issue raised in #134242, where Arc's documentation lacks Global Allocator safety descriptions for three APIs. And this was confirmed by @workingjubilee :

Wait, nevermind. I apparently forgot the increment_strong_count is implicitly A = Global. Ugh. Another reason these things are hard to track, unfortunately.

PR Description

This PR updates the document for the following APIs:

  • Arc::from_raw
  • Arc::increment_strong_count
  • Arc::decrement_strong_count

These APIs currently lack an important piece of documentation: the raw pointer must point to a block of memory allocated by the global allocator. This crucial detail is specified in the source code but is not reflected in the documentation, which could lead to confusion or incorrect usage by users.

Problem:

The following example demonstrates the potential confusion caused by the lack of documentation:

#![feature(allocator_api)]
use std::alloc::{Allocator,AllocError, Layout};
use std::ptr::NonNull;
use std::sync::Arc;

struct LocalAllocator {
    memory: NonNull<u8>,
    size: usize,
}

impl LocalAllocator {
    fn new(size: usize) -> Self {
        Self {
            memory: unsafe { NonNull::new_unchecked(&mut 0u8 as *mut u8) },
            size,
        }
    }
}

unsafe impl Allocator for LocalAllocator {
    fn allocate(&self, _layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
        Ok(NonNull::slice_from_raw_parts(self.memory, self.size))
    }

    unsafe fn deallocate(&self, _ptr: NonNull<u8>, _layout: Layout) {
    }
}

fn main() {
    let allocator = LocalAllocator::new(64);
    let arc = Arc::new_in(5, &allocator); // Here, allocator could be any non-global allocator
    let ptr = Arc::into_raw(arc);

    unsafe {
        Arc::increment_strong_count(ptr);
        let arc = Arc::from_raw(ptr);
        assert_eq!(2, Arc::strong_count(&arc)); // Failed here!
    }
}

@rustbot
Copy link
Collaborator

rustbot commented Dec 19, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ibraheemdev (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 19, 2024
@rustbot

This comment was marked as resolved.

Sign in to view
@rustbot

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jieyouxu jieyouxu removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 19, 2024
@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 21, 2024
@rustbot

This comment has been minimized.

Sign in to view
@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 21, 2024
@ibraheemdev
Copy link
Member

I'm not sure I understand your example. Creating an Arc from any pointer other than a pointer returned to into_raw is unsound anyways, so your example is unsound in the first call to from_raw. The issue here would be using Arc::new_in and then using Arc::from_raw instead of from_raw_in, or similar.

The change itself looks good to me, but could we link to the appropriate *_in method as well?

@DiuDiu777
Copy link
Contributor Author

@ibraheemdev
Well, the essence of this example is to highlight that the pointers used by from_raw and increment_strong_count can originate from non-Global allocators (a safety concern omitted in the documentation).

You're absolutely right—from_raw is already unsound in this case. But if the pointer originates from new_in with a non-Global allocator (as shown in my updated example), it results in undefined behavior (UB).

The change itself looks good to me, but could we link to the appropriate *_in method as well?

Regarding this issue, I think it would be helpful to explicitly mention Global in the changed parts. While the *_in methods specify that allocations need to use A, users who don't check the from_raw's source code might not realize that it implicitly uses A=Global.

Thanks for taking the time to review this. I hope this clarifies the issue and provides a reasonable update to the std doc.

@DiuDiu777 DiuDiu777 mentioned this pull request Jan 2, 2025
Merged
ibraheemdev
ibraheemdev reviewed Jan 16, 2025
View reviewed changes
@ibraheemdev
Copy link
Member

r=me after the one typo.

@DiuDiu777 @ibraheemdev
fix typo in library/alloc/src/sync.rs
48e671e 
Co-authored-by: Ibraheem Ahmed <ibraheem@ibraheem.ca>
@ibraheemdev
Copy link
Member

ibraheemdev commented Jan 16, 2025 •
edited
Loading

Thanks! @bors r+

@bors
Copy link
Collaborator

bors commented Jan 16, 2025

📌 Commit 48e671e has been approved by ibraheemdev

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 16, 2025
This was referenced Jan 16, 2025
Closed
Closed
Merged
@bors bors merged commit 87b3671 into rust-lang:master Jan 16, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 16, 2025
@DiuDiu777 DiuDiu777 mentioned this pull request Jan 21, 2025
Merged
@DiuDiu777 DiuDiu777 deleted the fix-doc branch March 10, 2025 02:19
@DiuDiu777 DiuDiu777 restored the fix-doc branch March 10, 2025 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibraheemdev ibraheemdev ibraheemdev left review comments

Assignees

@ibraheemdev ibraheemdev

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.86.0
Development

Successfully merging this pull request may close these issues.
6 participants
@DiuDiu777 @rustbot @rust-log-analyzer @ibraheemdev @bors @jieyouxu